Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Failing test with nasty HRs #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Failing test with nasty HRs #56

wants to merge 1 commit into from

Conversation

Soreine
Copy link

@Soreine Soreine commented Aug 31, 2018 •
edited
Loading

Hello, I'm opening this PR with a failing test, that yields the following result:

{ 
 attributes: '___',
 body: '----\n\nSome markdown following a 4 dashes horizontal rule\n\n\nSome more Markdown\n',
 frontmatter: '# Heading 1 followed by 3 _underscores_\n\n___'
}

for the following string:

----

Some markdown following a 4 dashes horizontal rule

---

# Heading 1 followed by 3 _underscores_

___

---

Some more Markdown

Here is what seems to be wrong about it:

  • I would expect attributes to always be a JSON, not a string.
  • I did not know a frontmatter could be elsewhere than the start of a document. Is there a way for me to specify that I want to ignore frontmatters that are not at the start of a document?

I don't know if this is an issue with the regex of front-matter to detect where a frontmatter is in the text, or if it is an issue with js-yaml for returning something that is not a JSON. Any help is appreciated.

@Soreine
Copy link
Author

Soreine commented Oct 22, 2018

Even if this PR is not merged in the end, any advice that could improve my understanding of YAML frontmatters would be greatly appreciated

@jxson
Copy link
Owner

jxson commented Nov 5, 2018

Sorry for the delayed response, I have been really short on spare time.

I'll have to dig into this some as I would like to avoid having a broken test be in the source, i.e. I want to fix the bug ;)

It might be possible to check the value in front-matter where the YAML is parsed, e.g. check the type around line 41 in index.js. If the value being returned from the parse call is the wrong type it can be worked around but ideally there should be fix upstream to the js-yaml library.

@Soreine
Copy link
Author

Soreine commented Nov 6, 2018 •
edited
Loading

I'll have to dig into this some as I would like to avoid having a broken test be in the source, i.e. I want to fix the bug ;)

Yes of course, the goal is to merge the test and a fix that goes with it :p

If the value being returned from the parse call is the wrong type it can be worked around but ideally there should be fix upstream to the js-yaml library.

I said I would expect attributes to always be an object, not a string. But the YAML specs seems to be a superset of JSON, and I think just a string is valid JSON. So maybe it's not js-yaml's responsibility to check for these cases.

Also, I said that I did not know a frontmatter could be elsewhere than the start of a document. But actually the YAML spec says that there can be as many --- delimited sections anywhere inside a document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Soreine @jxson